Re: ArchiveEntry optional arguments refactoring

From: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: ArchiveEntry optional arguments refactoring
Date: 2019-01-23 15:12:15
Message-ID: CA+q6zcV_-i09mogV0GOUnvC3HmBkP4Ni_Yh9tR6zTTD-JRSEpw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> On 2019-01-17 09:29:04 -0800, Andres Freund wrote:
> On 2019-01-17 10:23:39 -0500, Tom Lane wrote:
> > 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.

To make this discussion a bit more specific, I've created a patch of how it can
look like. All the arguments, except Archive, CatalogId and DumpId I've moved
into the ArchiveOpts structure. Not all of them could be empty before, but
anyway it seems better for consistency and readability. Some of the arguments
had empty string as a default value, I haven't changed anything here yet
(although this mixture of NULL and "" in ArchiveEntry looks a bit confusing).

As Andres mentioned above, for 578b229718e / the WITH OID removal and pg_dump
modification from pluggable storage thread, this patch reduces number of
changes, related to ArchiveEntry, from 70+ to just one.

Attachment Content-Type Size
0001-ArchiveOpts-structure.patch application/octet-stream 51.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fabien COELHO 2019-01-23 15:44:58 Re: pg_dump multi VALUES INSERT
Previous Message Tom Lane 2019-01-23 14:46:39 Re: Analyze all plans