Thread: pgsql: Fix pg_dump so pg_upgrade'ing an extension with simple opfamilie

Fix pg_dump so pg_upgrade'ing an extension with simple opfamilies works.

As reported by Michael Feld, pg_upgrade'ing an installation having
extensions with operator families that contain just a single operator class
failed to reproduce the extension membership of those operator families.
This caused no immediate ill effects, but would create problems when later
trying to do a plain dump and restore, because the seemingly-not-part-of-
the-extension operator families would appear separately in the pg_dump
output, and then would conflict with the families created by loading the
extension.  This has been broken ever since extensions were introduced,
and many of the standard contrib extensions are affected, so it's a bit
astonishing nobody complained before.

The cause of the problem is a perhaps-ill-considered decision to omit
such operator families from pg_dump's output on the grounds that the
CREATE OPERATOR CLASS commands could recreate them, and having explicit
CREATE OPERATOR FAMILY commands would impede loading the dump script into
pre-8.3 servers.  Whatever the merits of that decision when 8.3 was being
written, it looks like a poor tradeoff now.  We can fix the pg_upgrade
problem simply by removing that code, so that the operator families are
dumped explicitly (and then will be properly made to be part of their
extensions).

Although this fixes the behavior of future pg_upgrade runs, it does nothing
to clean up existing installations that may have improperly-linked operator
families.  Given the small number of complaints to date, maybe we don't
need to worry about providing an automated solution for that; anyone who
needs to clean it up can do so with manual "ALTER EXTENSION ADD OPERATOR
FAMILY" commands, or even just ignore the duplicate-opfamily errors they
get during a pg_restore.  In any case we need this fix.

Back-patch to all supported branches.

Discussion: <20228.1460575691@sss.pgh.pa.us>

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/6cead413bb92be0579a2dbf6320121edcc32e369

Modified Files
--------------
src/bin/pg_dump/pg_dump.c | 47 +----------------------------------------------
1 file changed, 1 insertion(+), 46 deletions(-)

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Fix pg_dump so pg_upgrade'ing an extension with simple opfamilies works.

I see this has broken the buildfarm's pg_upgrade checks, though only in
HEAD which makes me suspicious that it's exposing a pre-existing bug.
Odd, because I definitely tested pg_upgrade here.  Will dig into it
later this evening.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > Fix pg_dump so pg_upgrade'ing an extension with simple opfamilies works.
>
> I see this has broken the buildfarm's pg_upgrade checks, though only in
> HEAD which makes me suspicious that it's exposing a pre-existing bug.
> Odd, because I definitely tested pg_upgrade here.  Will dig into it
> later this evening.

My test shows, near the end:

ERROR:  zero-length delimited identifier at or near """" at character 40
STATEMENT:  CREATE OPERATOR FAMILY "box_ops" USING "";

Not immediately seeing why it's unable to find the amname for box_ops
though.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I see this has broken the buildfarm's pg_upgrade checks, though only in
>> HEAD which makes me suspicious that it's exposing a pre-existing bug.
>> Odd, because I definitely tested pg_upgrade here.  Will dig into it
>> later this evening.

> My test shows, near the end:

> ERROR:  zero-length delimited identifier at or near """" at character 40
> STATEMENT:  CREATE OPERATOR FAMILY "box_ops" USING "";

> Not immediately seeing why it's unable to find the amname for box_ops
> though.

It looks like there is indeed a pre-existing bug: create_am.sql creates
an index AM, creates an opfamily for it, and then drops the index AM ...
but yet the opfamily still remains.  Somebody (possibly me) missed the
boat on dependency creation.  This is likely a *very* longstanding bug,
but it never mattered until we had the ability to create/drop index AMs.

Should have a fix in a bit.

            regards, tom lane