Re: pg_migrator issue with contrib

I wrote:
> * pageinspect has changed the ABI of get_raw_page() in a way that will
> likely make it dump core if the function definition is migrated from
> an old DB. This needs to be fixed.
> [ and similarly for some other contrib modules ]

After thinking about this some more, I think that there is a fairly
simple coding rule we can adopt to prevent post-migration crashes
of the sort I'm worrying about above. That is:

* If you change the ABI of a C-language function, change its C name.

This will ensure that if someone tries to migrate the old function
definition from an old database, they will get a pg_migrator failure,
or at worst a clean runtime failure when they attempt to use the old
definition. They won't get a core dump or some worse form of security
problem.

As an example, the problem in pageinspect is this diff:

***************
*** 6,16 ****
--
-- get_raw_page()
--
! CREATE OR REPLACE FUNCTION get_raw_page(text, int4)
RETURNS bytea
AS 'MODULE_PATHNAME', 'get_raw_page'
LANGUAGE C STRICT;

--
-- page_header()
--
--- 6,21 ----
--
-- get_raw_page()
--
! CREATE OR REPLACE FUNCTION get_raw_page(text, text, int4)
RETURNS bytea
AS 'MODULE_PATHNAME', 'get_raw_page'
LANGUAGE C STRICT;

+ CREATE OR REPLACE FUNCTION get_raw_page(text, int4)
+ RETURNS bytea
+ AS $$ SELECT get_raw_page($1, 'main', $2); $$
+ LANGUAGE SQL STRICT;
+
--
-- page_header()
--
***************

The underlying C-level get_raw_page function is still there, but
it now expects three arguments not two, and will crash if it's
passed an int4 where it's expecting a text argument. But the old
function definition will migrate without error --- there's no way
for pg_migrator to realize it's installing a security hazard.

The way we should have done this, which I intend to go change it to,
is something like

CREATE OR REPLACE FUNCTION get_raw_page(text, int4)
RETURNS bytea
AS 'MODULE_PATHNAME', 'get_raw_page'
LANGUAGE C STRICT;

CREATE OR REPLACE FUNCTION get_raw_page(text, text, int4)
RETURNS bytea
AS 'MODULE_PATHNAME', 'get_raw_page_3'
LANGUAGE C STRICT;

so that the old function's ABI is preserved. Migration of the old
contrib module will then lead to the 3-argument function not being
immediately available, but the 2-argument one still works. Had we not
wanted to keep the 2-argument form for some reason, we would have
provided only get_raw_page_3 in the .so file, and attempts to use the
old function definition would fail safely.

(We have actually seen similar problems before with people trying
to dump and reload database containing contrib modules. pg_migrator
is not creating a problem that wasn't there before, it's just making
it worse.)

Comments, better ideas?

regards, tom lane