Date: Tue, 22 Jan 2019 15:07:30 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: A few new options for vacuumdb
Message-ID: <20190122060730.GD8719@paquier.xyz>
References: <33F59F9F-78A6-4ECE-BFE0-B1E4AAF40094@amazon.com>
 <579F2B3A-4EA5-4032-AAD9-4AEE9219A4AB@amazon.com>
 <CAD21AoDHsFx_Rnp-AB2UT2ORc3_PmCx18-wfrA_38i038D5WZQ@mail.gmail.com>
 <20190108020300.GH22498@paquier.xyz>
 <FF67D651-A7AF-4E70-9401-805E3CFB4DC9@amazon.com>
 <20190109010637.GC21835@paquier.xyz>
 <CAD21AoAqsSoTFWjajz1yycwxDnLZSWfV7wT7mmMBqy2k-jkLUQ@mail.gmail.com>
 <20190109043316.GH21835@paquier.xyz>
 <CAD21AoCzekiuDaGN-JqtoLgaYqqxGQ6nTcNqN0bm1EkvmcUKPA@mail.gmail.com>
 <5C971918-E566-41AB-895A-C4793D56B041@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="y2zxS2PfCDLh6JVG"
Content-Disposition: inline
In-Reply-To: <5C971918-E566-41AB-895A-C4793D56B041@amazon.com>
Precedence: bulk


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

On Mon, Jan 21, 2019 at 10:09:09PM +0000, Bossart, Nathan wrote:
> Here's a new patch set that should address the feedback in this
> thread.  The changes in this version include:
>=20
>  - 0001 is a small fix to the 'vacuumdb --disable-page-skipping'
>    documentation.  My suggestion is to keep it short and simple like
>    --full, --freeze, --skip-locked, --verbose, and --analyze.  The
>    DISABLE_PAGE_SKIPPING option is well-described in the VACUUM
>    documentation, and IMO it is reasonably obvious that such vacuumdb
>    options correspond to the VACUUM options.

Okay, committed this one.

>  - v3-0002 is essentially v2-0001 and v2-0004 combined.  I've also
>    added a comment explaining the importance of fully qualifying the
>    catalog query used to discover tables to process.

Regarding the search_path business, there is actually similar business
in expand_table_name_patterns() for pg_dump if you look close by, so
as users calling --table don't have to specify the schema, and just
stick with patterns.

+   /*
+    * Prepare the list of tables to process by querying the catalogs.
+    *
+    * Since we execute the constructed query with the default search_path
+    * (which could be unsafe), it is important to fully qualify everything.
+    */
It is not only important, but also absolutely mandatory, so let's make
the comment insisting harder on that point.  From this point of view,
the query that you are building is visibly correct.

+       appendStringLiteralConn(&catalog_query, just_table, conn);
+       appendPQExpBuffer(&catalog_query, "::pg_catalog.regclass\n");
+
+       pg_free(just_table);
+
+       cell =3D cell->next;
+       if (cell =3D=3D NULL)
+           appendPQExpBuffer(&catalog_query, " )\n");
Hm.  It seems to me that you are duplicating what
processSQLNamePattern() does, so we ought to use it.  And it is
possible to control the generation of the different WHERE clauses with
a single query based on the number of elements in the list.  Perhaps I
am missing something?  It looks unnecessary to export
split_table_columns_spec() as well.

-   qr/statement: ANALYZE;/,
+   qr/statement: ANALYZE pg_catalog\./,
Or we could just use "ANALYZE \.;/", perhaps patching it first.
Perhaps my regexp here is incorrect, but I just mean to check for an
ANALYZE query which begins by "ANALYZE " and finishes with ";",
without caring about what is in-between.  This would make the tests
more portable.

>  - 0003 includes additional documentation changes explaining the main
>    uses of --min-xid-age and --min-mxid-age and linking to the
>    existing wraparound documentation.

+$node->issues_sql_like(
+   [ 'vacuumdb', '--table=3Dpg_class', '--min-mxid-age=3D123456789',
'postgres'],
+   qr/AND GREATEST\(pg_catalog.mxid_age\(c.relminmxid\),
pg_catalog.mxid_age\(t.relminmxid\)\) OPERATOR\(pg_catalog.>=3D\) 123456789=
/,
+   'vacuumdb --table --min-mxid-age');
+$node->issues_sql_like(
+   [ 'vacuumdb', '--min-xid-age=3D987654321', 'postgres' ],
+   qr/AND GREATEST\(pg_catalog.age\(c.relfrozenxid\),
pg_catalog.age\(t.relfrozenxid\)\) OPERATOR\(pg_catalog.>=3D\) 987654321/,
+   'vacuumdb --min-xid-age');
It may be better to use numbers which make sure that no relations are
actually fetched, so as if the surrounding tests are messed up we
never make them longer than necessary just to check the shape of the
query.
--
Michael

--y2zxS2PfCDLh6JVG
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlxGsyIACgkQnvQgOdby
QH0pww//bMNfLaARKMig6Su0VwdYC4u5YuvGLMe1LMuUaMRW/0xcL8qlciSX1KVX
Ng5uuiWRmGBtbOXZkArT09yvIEhUMG+MpONIQmR1sDNl+AtKVURd782ltr4oIrPf
bR7L4GGgk2JkSk4NUe04E0EEnY1l9f51IGqFCMNyRnFj7zpaHmjYtShpJeqKNG/R
E/aKPCDKWpDuIhetTwR1XBSp8UxmKY3ZKIzvj3K+a+3dV8VOpGMN/Hz3kA722gH6
saOdp1jniFKxfJbQnKQey9ftFwzSHXrE2mbtL5XI9kB55FGPa++63N8x19N0Yk1c
4/btwYfY/hhFH3QuekTuUTbdavpYNaRLDdsF6vgufJNck+PVlbT1z/GrGx0nsfAi
dFsdB7GFJ8o322Nk2eV/tBIdxy3rnUfODJFV+v1CcGj0IYCzjXbGJyKiuUlHdrBH
QyKzjbpPoNJDMYpmm1KpUMpAktj/tp57PobeHZNk2Y2h7R81yjBl1k6WPGdFrVOP
FRZukhq6XCpy6fTPtEU6Dp8ruiEDlLhRc3cTCx2o3wxscRPujhkoZiXM3+XLfytw
87//ciyngaNR7GPsa5SWEV1zVxeOZF1c0hIwoAAydcu8EfPSePB+IBDZrJLNdaBA
amk1R1Km62z9crtYcg/hmhvCeAumVQVzF0m9+jtoQd8oyWrr1mg=
=tB5f
-----END PGP SIGNATURE-----

--y2zxS2PfCDLh6JVG--