Date: Thu, 17 Jan 2019 13:20:30 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Alvaro Herrera <alvherre@2ndquadrant.com>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: ArchiveEntry optional arguments refactoring
Message-ID: <20190117182030.GJ2528@tamriel.snowman.net>
References: <201901171502.nh6krqterjdd@alvherre.pgsql>
 <13281.1547738619@sss.pgh.pa.us>
 <20190117172904.ivmmrkxuiwv5lven@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="+p+ErxU3L3Rmijss"
Content-Disposition: inline
In-Reply-To: <20190117172904.ivmmrkxuiwv5lven@alap3.anarazel.de>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--+p+ErxU3L3Rmijss
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 =3D 3,
> > >> .dumpFn =3D 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.
>=20
> 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.
>=20
> 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);
>=20
> If you compare that with
>=20
> 	ArchiveEntry(AH,
>                  (ArchiveEntry){.catalogId =3D nilCatalogId,
>                                 .dumpId =3D createDumpId(),
>                                 .tag =3D "ENCODING",
>                                 .desc =3D "ENCODING",
>                                 .section =3D SECTION_PRE_DATA,
>                                 .defn =3D qry->data});
>=20
> 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 =3D palloc0(sizeof(JsonSemAction));

=2E..

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

        pg_parse_json(lex, sem);

=2E..

        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.
>=20
> Why? A lot of arguments that'd be potentially added or removed would not
> be set by each callsites.
>=20
> If you e.g. look at
>=20
> 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);
>=20
> i.e. just removing a 'false' argument. In like 70+ callsites. With the
> above scheme, we'd instead just have removed a single .withoids =3D 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

--+p+ErxU3L3Rmijss
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJcQMduAAoJEO1sijiDR2RVJKUQALWPGYdAjucupw+F/vPJkuns
brrCGiVPcNu2woggkJor5m76JLhtv18xqRfvBrjbnmpX76fxUVt2tYcEpiL5qiBP
UWbMoL9rkDSgRxP/R3rFrmEY92G7wHLN9fyhP3/q6fl15PppHTEcloE71igu2jrQ
94LPbCmlvkPbEvahq7R5pp96w79VUPzZGbRt1O25H7amsvZL8nQcSztXIcz95M6N
BqN6T8KQtfP47Ikc09MIpPl7UZC2nQ6AUIDnv/O6pGMcpGfH52ZNK/TsJkyMCAlP
irZkYU5h71GHuCyi8FpFImHVeeS4h8rNewYuEw4sVdfa7TmnTJ4NaI/DmtaMoUNT
LHglO0q2PNxUE7+3/sVygpfSQWJwzCAWmXkzUWAopgrdSoTQkYEM8dF9VKgOARbk
6T9cRlsq3CYhCdt6Qkv/WcVCRbcjVgiGOPw8lPEoCVAxz8U9hl8lZO0mZSkZ6VWa
8+QhpygBkeqJYOE8HciTlD7I4jR27kda7xUXjqmOv+4UQWtBopQmyLwnaW2+l5go
JczCfJGDIPMoZiGn4n5yuwV2xypckJei4XuzTEetZH77LzwE5VIcKpv3hq86ehRx
1xNwPHgMbdfAEb9owWBUIVqo5sRr5KRRMd42/Uy1obwy8UgNJhkR/Rb+uwEgtdPc
UsOpeGiOnNDhqSZ24njk
=UGxm
-----END PGP SIGNATURE-----

--+p+ErxU3L3Rmijss--