Date: Tue, 24 Jul 2018 13:14:03 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Jeremy Schneider <schnjere@amazon.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	"Albin, Lloyd P" <lalbin@scharp.org>
Subject: Re: BUG #15182: Canceling authentication due to timeout aka Denial
 of Service Attack
Message-ID: <20180724041403.GF4035@paquier.xyz>
References: <152512087100.19803.12733865831237526317@wrigleys.postgresql.org>
 <9145334b-9583-124c-012e-7d58039ad417@amazon.com>
 <CA+TgmobgJ9b0LpwhcCdj_gMDgp9UKmR_1pfX6WpWxGGGVrOG7A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="IvGM3kKqwtniy32b"
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmobgJ9b0LpwhcCdj_gMDgp9UKmR_1pfX6WpWxGGGVrOG7A@mail.gmail.com>
Precedence: bulk


--IvGM3kKqwtniy32b
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jul 23, 2018 at 11:29:33AM -0400, Robert Haas wrote:
> ExecuteTruncate needs to be refactored to use RangeVarGetRelidExtended
> with a non-NULL callback rather than heap_openrv, and
> expand_vacuum_rel needs to use RangeVarGetRelidExtended with a
> callback instead of RangeVarGetRelid.  See
> cbe24a6dd8fb224b9585f25b882d5ffdb55a0ba5 as an example of what to do.
> I fixed a large number of cases of this problem back around that time,
> but then ran out of steam and had to move onto other things before I
> got them all.  Patches welcome.

Thanks for pointing those out, I looked at both code paths recently for
some other work...  The amount of work does not consist just in using
for example RangeVarCallbackOwnsRelation for VACUUM and TRUNCATE.  There
are a couple of reasons behind that:
- While it would make sense, at least to me, to make VACUUM fall into if
allow_system_table_mods is allowed, that's not the case of ANALYZE as I
think that we should be able to call ANALYZE on a system catalog as
well.  So we would basically a new flavor of
RangeVarCallbackOwnsRelation for VACUUM which makes this difference
between vacuum and analyze with an argument in the callback, the options
of VacuumStmt would be nice.  This would not be used by autovacuum
anyway, but adding an assertion and mentioning that in the comments
would not hurt.  There is an argument for just restricting VACUUM FULL
as well and not plain VACUUM, as that's the one hurting here.
- TRUNCATE is closer to a solution, as it has its own flavor of relation
checks with truncate_check_rel.  So the callback would replace
truncate_check_rel but CheckTableNotInUse should be moved out of it.
TRUNCATE already uses allow_system_table_mods for its checks.

Thoughts?
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAltWp4sACgkQnvQgOdby
QH3rJhAAghUxGLx1XhnBRmrfg5zke8bK3n1H1wB/KLrgzjC8Y+phLcxDjQoCrZiy
fkmSUU83S6cJ25bIItv70bLk8fAVBr+BzeVUw7Mkl7ND1lAW0x5e/XrArFVnoYb5
Q6FSaRZLSNXlembOslsFau3DGd4Ojo0GL+iHB+VhBXOJDs3+mnxj2iv8YVRRDvzN
t2j8I2OHdvewoHAo7D3LI5JWmhXuydm8M43PVbWNalWBMprbUidy0MzthToblILn
fftn0a/eRuxWV0muxCSU7ZiukwXMTusRi6tYG3BN2RADxNhzIjuge8md0GJb39c+
R/b7kC970yMCi/ywbNHmzBs2w5R9SxMv4FDSHZrybbyQjF73vxZZKKG8GiuJgBFR
9M0y536PQpfkc46O0h4OsE+8gbKdZMJSNXxYNW7YsUrgIz1tGKSpSUDFbpQ+dQae
Uy0oNftsPgBWSYJO5fMZvpcnH9OxniSWc+rIqeY2pFt6tl+QE6A4zv3Y2tSoAJme
xPbp/IGucUbdkJSq5Zn3DwKPteA3YOkOcDoAswgUvlE7ZiLxZK+eARe6pQPEh0B3
VwKVE70Y8HZPVxbC61LIkZfduPYdG2VwcQYOHT+13cO8IZ2PS8HwC6WuCgWdzZWy
t2WaEMlGDahShgfc5jyoz5yUI3C4Wf5XMhHBT3IiEra2WARMDC4=
=AlUz
-----END PGP SIGNATURE-----

--IvGM3kKqwtniy32b--